Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[INTEL MKL] Fix proxy info during docker builds #29560

Merged
merged 6 commits into from
Jun 19, 2019

Conversation

ashahba
Copy link
Contributor

@ashahba ashahba commented Jun 8, 2019

This was causing swift package downloads to fail because for some reason HTTP_PROXY and HTTPS_PROXY values were empty.

@tensorflow-bot tensorflow-bot bot added the size:XS CL Change Size: Extra Small label Jun 8, 2019
@rthadur rthadur self-assigned this Jun 10, 2019
@rthadur rthadur requested a review from penpornk June 10, 2019 01:00
@rthadur rthadur added this to Assigned Reviewer in PR Queue via automation Jun 10, 2019
@rthadur rthadur added the comp:mkl MKL related issues label Jun 10, 2019
@ashahba
Copy link
Contributor Author

ashahba commented Jun 11, 2019

Hi @penpornk any feedback on this?

Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay!
I'm not sure why either, but adding these doesn't hurt. Thank you!

@tensorflow-bot tensorflow-bot bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jun 11, 2019
PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Jun 11, 2019
Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, please add a comment (in the docker file) why they are added. For future reference.

PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Jun 11, 2019
@penpornk penpornk removed the ready to pull PR ready for merge process label Jun 11, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jun 11, 2019
@ashahba
Copy link
Contributor Author

ashahba commented Jun 13, 2019

Thanks @penpornk
Comments added to the Dockerfile.

Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the changes! I was referring to why the uppercase letter ones need to be added. But additional info is great too! Sorry for being unclear.

TF_DOCKER_BUILD_ARGS+=("--build-arg HTTP_PROXY=${http_proxy}")
TF_DOCKER_BUILD_ARGS+=("--build-arg SOCKS_PROXY=${socks_proxy}")
TF_DOCKER_BUILD_ARGS+=("--build-arg NO_PROXY=${no_proxy}")
TF_DOCKER_BUILD_ARGS+=("--build-arg HTTP_PROXY=${HTTP_PROXY}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment explaining that we need the uppercase versions because the lowercase ones don't cover all the case (or something).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @penpornk to best of my knowledge I added a comment to describe why we need both set of proxies 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much, @ashahba! :)

@rthadur rthadur requested a review from penpornk June 14, 2019 21:45
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Jun 16, 2019
Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your patience!

TF_DOCKER_BUILD_ARGS+=("--build-arg HTTP_PROXY=${http_proxy}")
TF_DOCKER_BUILD_ARGS+=("--build-arg SOCKS_PROXY=${socks_proxy}")
TF_DOCKER_BUILD_ARGS+=("--build-arg NO_PROXY=${no_proxy}")
TF_DOCKER_BUILD_ARGS+=("--build-arg HTTP_PROXY=${HTTP_PROXY}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much, @ashahba! :)

@tensorflow-bot tensorflow-bot bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jun 16, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jun 16, 2019
@tensorflow-copybara tensorflow-copybara merged commit 798109d into tensorflow:master Jun 19, 2019
PR Queue automation moved this from Approved by Reviewer to Merged Jun 19, 2019
tensorflow-copybara pushed a commit that referenced this pull request Jun 19, 2019
@ashahba ashahba deleted the ashahba/proxy branch June 19, 2019 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes comp:mkl MKL related issues ready to pull PR ready for merge process size:XS CL Change Size: Extra Small
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

6 participants